[None][infra] Bump xgrammar#12811
Conversation
Signed-off-by: Yuanjing Xue <197832395+yuanjingx87@users.noreply.github.com>
📝 WalkthroughWalkthroughUpdated the xgrammar dependency from version 0.1.25 to 0.1.32 across build and Python configuration files. Added a patch file that applies constexpr optimizations to GrammarFunctor flag constants during the build process. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
3rdparty/CMakeLists.txt (1)
111-123: Patch command error handling may mask real failures.The current logic uses
||to catch any failure, but this could mask genuine patch application errors. If the dry-run succeeds but the actualpatchcommand fails for reasons other than "already applied" (e.g., file corruption, permission issues), the error is silently swallowed with a misleading message.🛠️ Suggested improvement for more robust error handling
- PATCH_COMMAND - bash -c "patch -p1 --forward --batch --dry-run -i '${_patch_file}' && \ - patch -p1 --forward --batch -i '${_patch_file}' || \ - echo 'Patch already applied, skipping.'") + PATCH_COMMAND + bash -c "if patch -p1 --forward --batch --dry-run -i '${_patch_file}' 2>/dev/null; then \ + patch -p1 --forward --batch -i '${_patch_file}'; \ + else \ + echo 'Patch already applied or not applicable, skipping.'; \ + fi")This ensures that if the dry-run fails (patch already applied), we skip gracefully, but if the dry-run succeeds and the actual patch fails, the error propagates correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@3rdparty/CMakeLists.txt` around lines 111 - 123, The PATCH_COMMAND currently swallows real patch failures by using "&& ... || echo", so replace that shell compound with an explicit conditional that runs the dry-run then, only if the dry-run succeeds, runs the real patch and lets its error propagate; otherwise print "Patch already applied, skipping." Update the PATCH_COMMAND (referencing _patch_file and the FetchContent_Declare for xgrammar) to use: bash -c "if patch -p1 --forward --batch --dry-run -i '${_patch_file}'; then patch -p1 --forward --batch -i '${_patch_file}'; else echo 'Patch already applied, skipping.'; fi" so dry-run failures skip but actual patch failures are not masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@3rdparty/CMakeLists.txt`:
- Around line 111-123: The PATCH_COMMAND currently swallows real patch failures
by using "&& ... || echo", so replace that shell compound with an explicit
conditional that runs the dry-run then, only if the dry-run succeeds, runs the
real patch and lets its error propagate; otherwise print "Patch already applied,
skipping." Update the PATCH_COMMAND (referencing _patch_file and the
FetchContent_Declare for xgrammar) to use: bash -c "if patch -p1 --forward
--batch --dry-run -i '${_patch_file}'; then patch -p1 --forward --batch -i
'${_patch_file}'; else echo 'Patch already applied, skipping.'; fi" so dry-run
failures skip but actual patch failures are not masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23334802-fc91-4080-a374-c17b6bb7fce8
📒 Files selected for processing (4)
3rdparty/CMakeLists.txt3rdparty/patches/xgrammar_constexpr.patchrequirements.txtsecurity_scanning/pyproject.toml
|
/bot run --disable-fail-fast |
|
PR_Github #42181 [ run ] triggered by Bot. Commit: |
Signed-off-by: Yuanjing Xue <197832395+yuanjingx87@users.noreply.github.com>
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #42187 [ run ] triggered by Bot. Commit: |
yuanjingx87
force-pushed
the
user/yuanjingx/cherry-pick-vulnerability-fix
branch
from
c2d6bb7 to
b07c6a7
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #42187 [ run ] completed with state
|
|
PR_Github #42196 [ run ] triggered by Bot. Commit: |
yuanjingx87
force-pushed
the
user/yuanjingx/cherry-pick-vulnerability-fix
branch
from
b07c6a7 to
a107811
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #42196 [ run ] completed with state
|
|
PR_Github #42206 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #42265 [ run ] triggered by Bot. Commit: |
|
PR_Github #42265 [ run ] completed with state |
4 participants
Summary by CodeRabbit
Description
Bump xgrammar
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.